-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Abstract browser
and browserType
from JS API
#910
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏
b api.Browser | ||
) | ||
|
||
if b, ok = vu.getBrowser(id); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're building a new browser
object on every iteration, so this would always return nil
and false
, no? If that's the case then maybe we don't need this check.
Now i see why -- calls to the other browser APIs will retrieve the associated browser
object (e.g. i can call newContext
multiple times and only the first call to newContext
will create the browser
object).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it's LGTM, thanks! I believe we need more tests for this change. Especially for reusing of browsers: getOrInitBrowser
. It would also be great if we could have tests for the contexts as they were the primary suspects of earlier bugs.
BrowserType will no longer be an exposed type through Goja layer, instead we will handle its functionality from our implementation in the Go layer.
Adds a new browser registry to be held at the root module level, which lifecycle spans throughout the whole test run. The registry will store browser instances per iteration so we can handle their lifecycle from Go layer based on the iteration lifecycle, which is tight to the VU context.
Retrieves the scenario name for the given VU context. This is required in order to build the key for the browsers pool, as the VU iteration is just a monotonic integer that is shared per scenario. Therefore, if we don't want to share the browser instance per scenario, we have to build the pooll key based on the scenario name also.
Because browser is not controlled by explicit JS test code, now every browser level method will automatically initialize the browser for the iteration in case it has not been initialized yet in a previous call. Once the browser is initialized it's stored in the browser registry indexed by the iteration ID (composed by VUID-scenario-iterationID). When the iteration ends, signaled by the VU context being done, the browser is automatically closed and removed from the registry.
554dd49
to
5ccd4f9
Compare
Because we want to handle browser lifecycle automatically from our Go implementation, we can not build the browser context based on the VU context, which will be closed at the end of every iteration. That would result in a race condition between iteration and browser closing procedure. Instead we will wait for the VU context to be closed and then from our implementation explicitely call Browser.Close() method.
Previously, the browser implementation components, such as browser, page, frame... were holding a context inherited from the vu.context because the browser lifecycle was tight to it. Therefore, we were passing these contexts when calling k6metrics.PushIfNotDone, as if that context was done, it meant that the VU context was also done and at the same time the VU metrics samples channel was closed. With the current implementation, because the browser, page, frame... contexts do not inherit from the VU.context, we can not use these contexts when calling k6metrics.PushIfNotDone, as it is not a guarantee that the vu samples channel is open, so we should use the VU context directly.
5ccd4f9
to
1a91e75
Compare
Merging this as it is. We will improve it once we have the k6 event system. |
A decision was made to no longer expose browser.on method as it currently only supports 'disconnect' event, and that is non applicable after the changes made to abstract the browser initialization/closing procedures from the JS API in #910.
A decision was made to no longer expose browser.on method as it currently only supports 'disconnect' event, and that is non applicable after the changes made to abstract the browser initialization/closing procedures from the JS API in #910.
Description of changes
This PR abstracts
browser
andbrowserType
implementations from the JS API. This will allow reducing the boilerplate code typically present in k6 browser scripts in order to initialize and close the browser object. Instead this will be handled transparently for the user.With this change we are also moving into a tighter integration with k6 by:
Checklist
Tasks
Optional tasks
Tasks